bindspace→mailbox_soa W1c: declared populated-row count (prefilter-bound fix)#519
Conversation
…d fix) The fix the 3-brutal-critic pass flagged (PP-13 P1-3): a zeroed MetaWord passes MetaFilter::accepts (0>=0, thinking_mask==0 accepts all), so a row-bounded sweep clamped to the capacity N (e.g. 1024) returns N−len phantom rows vs a BindSpace window of len. The migration read-shim's meta_prefilter analogue MUST clamp to a logical row count, not n_rows(). - MailboxSoA<N> gains `populated: usize` + `populated()` / `set_populated()` (clamped to N). Semantics mirror BindSpace::len: a DECLARED size set once (BindSpace::zeros(len) fixes len), NOT a per-write high-water-mark and NOT shrunk by reset_row. Defaults to 0 (empty mailbox). - Additive only; nothing deleted; BindSpace untouched. - test_mailbox_soa_populated_is_declared_len_not_capacity: default 0; n_rows()=N distinct from populated(); reset_row does not shrink; set_populated clamps to N. 17 mailbox_soa tests green, clippy clean. Unblocks W2 (the differential harness clamps its mailbox prefilter to populated()). Per the consolidated+critiqued wiring plan (bindspace-mailbox-soa-wiring-v1.md, v2). Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 53 minutes and 54 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b944bf24b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mb.n_rows(), | ||
| 1024, | ||
| "n_rows() is the capacity N, distinct from populated()" |
There was a problem hiding this comment.
This test cements n_rows() as capacity even after set_populated(4), but the contract trait documents MailboxSoaView::n_rows as the populated row count and downstream generic readers like SoaWavePrimer::project bound their row loop with soa.n_rows(). In the populated < N migration case this means any code receiving MailboxSoA through the trait still scans rows 4..1023, so the zeroed MetaWord phantom-row divergence this change is meant to prevent remains for the shared view surface; n_rows() (or an equivalent trait-visible bound) needs to reflect self.populated before callers can safely clamp through MailboxSoaView.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 018b0cd5 — and you caught a genuine contract-alignment bug, not just a test nit. The contract MailboxSoaView::n_rows() is documented "Number of populated rows in the SoA", but the MailboxSoA impl returned the capacity N, so generic view consumers (markov_soa / SoaWavePrimer::project, which bound 0..soa.n_rows()) scanned the zeroed padding rows populated..N through the shared trait surface.
MailboxSoaView::n_rows() now returns self.populated, aligning the impl with its own doc. populated() (inherent) and n_rows() (trait) now agree; N stays the type-level capacity const; edges_raw()/meta_raw() still return the full backing slice (consumers bound by n_rows(), the documented 0..n_rows() pattern). W1c test updated (n_rows() tracks populated(): 0 empty → 4 after set_populated(4) → 1024 after clamp). 17 mailbox_soa tests green incl. the scheduler driving loop; clippy clean. markov_soa is cross-crate generic (never receives a real MailboxSoA) so unaffected.
Generated by Claude Code
…apacity N Codex P1: the contract MailboxSoaView::n_rows() is documented "Number of populated rows in the SoA", but the MailboxSoA impl returned the const capacity N. Generic view consumers (e.g. SoaWavePrimer::project / markov_soa, which bound their row loop with soa.n_rows()) therefore scanned the zeroed padding rows populated..N through the shared trait surface — the exact phantom-row divergence W1c's inherent populated() prevents for the migration read-shim but NOT for the view surface. Fix: MailboxSoaView::n_rows() now returns self.populated (aligning the impl with its own documented contract). populated() (inherent) and n_rows() (trait) now agree; N stays the type-level capacity const. edges_raw()/meta_raw() still return the full backing slice (consumers bound by n_rows(), the documented pattern). W1c test updated to assert n_rows() tracks populated() (0 when empty, 4 after set_populated(4), 1024 after clamp). 17 mailbox_soa tests green incl. the scheduler driving loop; clippy clean. markov_soa is cross-crate generic (never receives a real MailboxSoA) so unaffected. Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cognitive-shader-driver/src/mailbox_soa.rs`:
- Around line 168-169: Update the stale documentation comments in the
mailbox_soa.rs file at the specified locations (lines 168-169, 232-233, and
306-307) to accurately reflect the current implementation. The comments
currently incorrectly describe n_rows() as returning capacity N and suggest that
writes automatically bump the population counter. Instead, revise the wording to
clarify that n_rows() now returns the declared populated value (not the
capacity), that no write path implicitly updates population, and that callers
must explicitly manage population tracking. Ensure the language makes clear that
n_rows() is semantically bound to the populated field and emphasize the manual
population management requirement to prevent future misuse.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe2ad397-245c-4939-9362-33fea5ea69c6
📒 Files selected for processing (1)
crates/cognitive-shader-driver/src/mailbox_soa.rs
…bit) After the codex P1 fix bound MailboxSoaView::n_rows() to `populated` (no longer the const capacity N), three doc sites still described the OLD semantics. CodeRabbit (PR #519) flagged all three as a doc/code mismatch that would breed future misuse: - field doc (~L168): "MUST clamp to populated, never to n_rows() (= N)" was self-contradictory now that n_rows() == populated(). Reworded: clamp to the logical populated count, not the type-level capacity N; noted n_rows() is now bound to populated so either is safe, only N is wrong. - new() init comment (~L232): "until a write bumps the mark" was false (no write path bumps populated implicitly). Reworded to "until set_populated(...) declares the size". - populated() getter doc (~L305): "clamp to this, never to n_rows()" was stale. Reworded to the logical-size framing and made the manual-declaration (never per-write-counter) requirement explicit. Doc-only; semantics unchanged. 17 mailbox_soa tests green, clippy clean, no broken intra-doc links. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CcpLeEC3XK8Eye53GKBVvi
2 participants
What
W1c of the bindspace→mailbox_soa arc — adds
MailboxSoA::populated(), the prefilter-bound fix the 3-brutal-critic pass surfaced. Strictly additive, tested, nothing deleted.The bug it pre-empts (PP-13 P1-3)
A zeroed
MetaWordpassesMetaFilter::accepts(0 >= 0,thinking_mask == 0accepts all). So a row-bounded sweep (the migration read-shim'smeta_prefilteranalogue) clamped to the capacityN(e.g. 1024) would return N − len phantom rows versus aBindSpacewindow oflen— silent divergence in the W2 differential.MailboxSoApreviously had no logical-size field (n_rows()returns the constN).Change (
crates/cognitive-shader-driver/src/mailbox_soa.rs)MailboxSoA<N>gainspopulated: usize+populated()/set_populated()(clamped toN).BindSpace::len: a declared size set once (just asBindSpace::zeros(len)fixeslen) — not a per-write high-water-mark, and not shrunk byreset_row(clearing a row's contents doesn't changelen). Defaults to0.populated(), nevern_rows().Test (17
mailbox_soagreen, clippy clean)test_mailbox_soa_populated_is_declared_len_not_capacity: default0;n_rows()=Ndistinct frompopulated();reset_rowdoes not shrink it;set_populatedclamps toN.Why now
Unblocks W2 (the differential harness builds a mailbox with
set_populated(len)and clamps its prefilter topopulated(), so it sweeps exactly the rows aBindSpacewindow oflenwould). Per the consolidated + 3-brutal-critic-hardened wiring plan (bindspace-mailbox-soa-wiring-v1.md, v2). Column migration (W1+W1b) is already onmain; this is the last small additive prerequisite before the W2 differential and the W3+W4a atomic read/write re-point.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit